feat: add retry mechanism and configurable interval for update checks #201
Conversation
Unit Test Results10 tests 10 ✅ 1s ⏱️ Results for commit fc004dc. ♻️ This comment has been updated with latest results. |
src/main/java/net/onelitefeather/antiredstoneclockremastered/service/UpdateService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds retry logic and configurable interval for the update check service. The changes introduce a retry mechanism that stops checking for updates after a maximum number of failed attempts and makes the update check interval configurable via system properties.
- Adds configurable update check interval via system property
ARCR_UPDATE_SERVICE_SCHEDULE - Implements retry count mechanism with configurable max retries via system property
ARCR_UPDATE_SERVICE_MAX_RETRY - Adds HTTP status code validation to detect and count failed update checks
Comments suppressed due to low confidence (1)
src/main/java/net/onelitefeather/antiredstoneclockremastered/service/UpdateService.java:106
- When an IOException or InterruptedException occurs, the retry count is not incremented, but it is incremented for non-200 status codes. This inconsistency means network failures (exceptions) don't count toward the max retry limit, while HTTP errors do. Consider incrementing
retryCount++in the catch block to ensure all failures are counted consistently.
} catch (IOException | InterruptedException e) {
LOGGER.error("Something went wrong to check updates", e);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (retryCount >= MAX_RETRY_COUNT) { | ||
| LOGGER.error("Max retry count reached for update check, stopping further attempts"); | ||
| LOGGER.error("Please check your internet connection or https://hangar.papermc.io/ status."); | ||
|
|
There was a problem hiding this comment.
The early return when max retry count is reached prevents the scheduled task from continuing. However, the task will continue to run on its schedule even after max retries are reached, causing this code block to execute repeatedly. Consider canceling the scheduler when max retries are reached using this.scheduler.cancel() to prevent unnecessary repeated checks.
| if (this.scheduler != null) { | |
| this.scheduler.cancel(); | |
| } |
| private final SchedulerService schedulerService; | ||
| private Version remoteVersion; | ||
| private ScheduledTask scheduler; | ||
| private int retryCount = 0; |
There was a problem hiding this comment.
The retryCount field is accessed from the asynchronous scheduled task without synchronization. Multiple scheduled tasks could potentially execute concurrently, leading to race conditions when reading and incrementing retryCount. Consider using AtomicInteger instead of int for thread-safe operations.
| LOGGER.error("Failed to check for updates, status code: {}", httpResponse.statusCode()); | ||
| retryCount++; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Successful update checks (status code 200) that don't find a newer version don't reset the retry count. This means if the service experiences 4 failures followed by repeated successful checks that find no updates, it will stop working after one more failure. Consider resetting retryCount = 0 when a successful response is received (status code 200) to reset the failure counter.
| } | |
| } | |
| retryCount = 0; |
Proposed changes
We added a status code check and also a retry config to improve stability.
Fixes #196
Types of changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any ofthem, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before
merging your code.
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you
did and what alternatives you considered, etc...